Testing and clarifying shared unique key requirement #358
Conversation
| @@ -31,7 +31,9 @@ The `SUPER` privilege is required for `STOP SLAVE`, `START SLAVE` operations. Th | |||
| - MySQL 5.7 `JSON` columns are not supported. They are likely to be supported shortly. | |||
|
|
|||
| - The two _before_ & _after_ tables must share some `UNIQUE KEY`. Such key would be used by `gh-ost` to iterate the table. | |||
| - As an example, if your table has a single `UNIQUE KEY` and no `PRIMARY KEY`, and you wish to replace it with a `PRIMARY KEY`, you will need two migrations: one to add the `PRIMARY KEY` (this migration will use the existing `UNIQUE KEY`), another to drop the now redundant `UNIQUE KEY` (this migration will use the `PRIMARY KEY`). | |||
| - What matters is not the key's _name_, but the columns covered by such key. | |||
| - As an example, if your table has a single `UNIQUE KEY my_unique_key(list, of, columns)` and no `PRIMARY KEY`, and you wish to replace the unique key with a `PRIMARY KEY`, you are good to go with a single migration: `--alter='DROP KEY my_unique_key, ADD PRIMARY KEY (list, of, columns)'` | |||
There was a problem hiding this comment.
Depending on the outcome of testing, you should add what happens if you replace a PK (since the issue that sparked this got the PK demoted to unique and replaced with a different key). E.g. is it ok to switch out the PK if the original PK becomes a unique key? (Not necessarily obvious).
| @@ -30,8 +30,7 @@ The `SUPER` privilege is required for `STOP SLAVE`, `START SLAVE` operations. Th | |||
|
|
|||
| - MySQL 5.7 `JSON` columns are not supported. They are likely to be supported shortly. | |||
|
|
|||
| - The two _before_ & _after_ tables must share some `UNIQUE KEY`. Such key would be used by `gh-ost` to iterate the table. | |||
| - As an example, if your table has a single `UNIQUE KEY` and no `PRIMARY KEY`, and you wish to replace it with a `PRIMARY KEY`, you will need two migrations: one to add the `PRIMARY KEY` (this migration will use the existing `UNIQUE KEY`), another to drop the now redundant `UNIQUE KEY` (this migration will use the `PRIMARY KEY`). | |||
| - The two _before_ & _after_ tables must share some `UNIQUE KEY`. Such key would be used by `gh-ost` to iterate the table. See [Read more](shared-key.md) | |||
|
|
|||
| - The chosen migration key must not include columns with `NULL` values. | |||
There was a problem hiding this comment.
I feel this is a sub-note of the previous note... It is about the migration key.
I might even want to say "the chosen migration key must include only not null columns". "columns with null values" could be more vague than referring to the column description being NOT NULL?
There was a problem hiding this comment.
I might even want to say "the chosen migration key must include only not null columns".
However that is not the case. By default gh-ost does indeed only look for Unique Keys with not null columns, but supports null-able columns when the user takes responsibility and provides with --allow-nullable-unique-key.
For the migration to run correctly there must be no null values in the chosen key. gh-ost utilizes table definition when possible to know that is not the case, or trusts the user when this is not the case.
There was a problem hiding this comment.
Okay. I think this is really wordy, though, because it confused me. Since we're doing things in note form, maybe something like:
-
The migration key must not include columns with
NULLvalues. This means either:- The columns are
NOT NULL, or - The columns are nullable but don't contain any NULL values.
- The columns are
-
gh-ostwill try to pick a migration key with non-nullable columns. It will not run if the onlyUNIQUE KEYincludes nullable columns.- You may override this via
--allow-nullable-unique-keybut make sure there are no actualNULLvalues in those columns. ExistingNULLvalues can't guarantee data integrity on the migrated table.
- You may override this via
There was a problem hiding this comment.
This is great wording and better than I can express myself. Would you mind pushing this into this branch?
Also, I don't know if you've seen the new file, shared-key.md? It really elaborates on the topic. I'm thinking we can just drop the entire paragraph from the requirements page altogether, given that it links to that page, and add your description in the shared-key.md file?
There was a problem hiding this comment.
Yeah, I saw shared-key.md too... The shared unique key and non-null values should be in the main page, I think. Can look at a moving around of text when it's not my bedtime.
There was a problem hiding this comment.
I made an edit but maybe it's still too wordy and I should have edited shared-key.md.
|
Ok to merge for now. |
Storyline: #357
This PR adds the tests to confirm expected behavior of
gh-ost, when requiring shared unique key between original/ghost tables.To be followed up by a documentation update.
cc @ggunson